Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: 7093 add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again #7169

Merged

Conversation

drijkersbq
Copy link
Contributor

@drijkersbq drijkersbq commented Nov 19, 2024

fix: add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again

Fixes Issue

7093

Description of Change

Added username and password properties for authentication of both the central.content.url and analyzer.central.url:

  • analyzer.central.username
  • analyzer.central.password
  • central.content.username
  • central.content.password

This fixes the problem that the new httpclient doesn't support userinfo components in these url's anymore.

Have test cases been added to cover the new functionality?

no

… for central.content.url and analyzer.central.url again
@aikebah
Copy link
Collaborator

aikebah commented Nov 20, 2024

Thanks for the PR.

As it adds CLI commandline arguments cli/src/site/markdown/arguments.md should also be updated to make documentation reflect the new arguments.

It would be worthwhile to also add the same configurability to the maven plugin and Ant task (and the gradle plugin, which is maintained in a different repo), but I'm perfectly fine in enhancing those as separate PRs

@aikebah aikebah added this to the 11.2.0 milestone Nov 20, 2024
@boring-cyborg boring-cyborg bot added the documentation site documentation label Nov 21, 2024
@drijkersbq
Copy link
Contributor Author

I have added the two arguments that are added to the cli to the arguments.md as requested

@aikebah
Copy link
Collaborator

aikebah commented Nov 21, 2024

@drijkersbq Overlooked in my initial review, but spotted it after you added the documentation: In the settings you introduce properties to override user/pass for search as well as for content. But in the CLI arguments you only add a way to override the credentials for search. Intentional? Or accidental because credentials and host happen to be identical in your case so that defining them once makes both cases succeed for you?

If host and port are the same one set of creds would replace the other, as the credentials put in a map linked to host/port combo, which means that if both are the same for search- and content-URL it would likely go unnoticed that only one of the two was attempted to be configured (unless the proxy host was expecting different credentials for search vs content paths, in which case one of the two would still fail, but that would be an unsupported case with the current codebase anyhow given that neither URI-paths nor authentication-realms play a role in the current configs for credentials)

@drijkersbq
Copy link
Contributor Author

@aikebah The reason why i've only added CLI arguments for just one of the two url's is because only one of those two url's was currently available as a CLI argument itself. The second url was not yet available as cli argument.

Not knowing why the second url wasnt available as cli argument already (perhaps there was a good reason for it), i didnt it's new credential properties to the cli.

It's true in our case both url's are thesame so we could get away with only one url+credentials-set in the cli arguments. However we run the project with a custom property file where we set the corresponding properties for both url's, so we dont use the cli arguments for either of the urls.

If you want i can add the second url and its new credential properties to the cli arguments to match up the first url.

@aikebah
Copy link
Collaborator

aikebah commented Nov 22, 2024

@drijkersbq No change needed, would be fine as is... was reviewing on iPad, so not easy to cross-check on codebase. Your explanation makes sense.

LGTM

Copy link
Collaborator

@aikebah aikebah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aikebah
Copy link
Collaborator

aikebah commented Nov 22, 2024

@jeremylong would appreciate your review as well before merging it in.

Copy link
Owner

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeremylong jeremylong merged commit 20e344c into jeremylong:main Dec 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli changes to the cli documentation site documentation utils changes to utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants